Skip to content

Python: dataflow, unify iterated unpacking#5047

Merged
tausbn merged 7 commits into
github:mainfrom
yoff:python-dataflow-unpacking-unifying-experiments
Feb 4, 2021
Merged

Python: dataflow, unify iterated unpacking#5047
tausbn merged 7 commits into
github:mainfrom
yoff:python-dataflow-unpacking-unifying-experiments

Conversation

@yoff

@yoff yoff commented Jan 28, 2021

Copy link
Copy Markdown
Contributor

This PR adds a for read-step and makes the comprehension read-step obsolete.
It also adds a for read-step targetting sequences.

read-step. Add a version targetting sequence nodes.
@yoff yoff force-pushed the python-dataflow-unpacking-unifying-experiments branch from 9d7019e to 182d435 Compare January 29, 2021 16:39
@yoff yoff marked this pull request as ready for review January 29, 2021 16:47
@yoff yoff requested a review from a team as a code owner January 29, 2021 16:47
Comment thread python/ql/src/semmle/python/dataflow/new/internal/DataFlowPrivate.qll Outdated

@tausbn tausbn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable. Just a few comments and questions.

Comment thread python/ql/src/semmle/python/dataflow/new/internal/DataFlowPrivate.qll Outdated
…ate.qll

Co-authored-by: Taus <tausbn@github.com>
@yoff yoff requested a review from tausbn February 1, 2021 15:41

@RasmusWL RasmusWL left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me, although I'm surprised we're not able to handle more comprehensions

def test_unpacking_comprehension():
x = [a for (a, b) in [(SOURCE, NONSOURCE)]]
SINK(x[0]) # Flow missing
SINK(x[0]) #$ MISSING:flow="SOURCE, l:-1 -> x[0]"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised we're not able to handle this one now. can you explain why? 😕

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I had missed this bit..

yoff and others added 2 commits February 3, 2021 21:56
…ate.qll

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
@yoff yoff requested a review from RasmusWL February 3, 2021 21:16

@RasmusWL RasmusWL left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM now 👍

@tausbn tausbn merged commit 634041d into github:main Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants